-
Notifications
You must be signed in to change notification settings - Fork 607
Add dotnet bindings to Turso #3594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
alsi-lawr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be 2 separate libraries to have proper separation of concerns: 1. A raw bindings library (think SQLiteRawPCL, like Turso.Raw) that has the P/Invoke calls internal to the library and a publicly shipped API that maps the unmanaged types to managed C# types (where appropriate).
2. The actual ADO.NET provider for turso
I'd suggest something like Turso.Raw and Turso.Data for the two libraries. There's a lot of misunderstanding of how to safely do interop between unmanaged and managed langs, but that should be relatively straight forward to iron out.
| using var command = connection.CreateCommand(); | ||
| command.CommandText = "SELECT * FROM t;"; | ||
| using var reader = command.ExecuteReader(); | ||
| while (reader.Read()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JIT is very likely to completely optimize out most of this loop since the reader.Read values are just being discarded. You have to do something with the value to force the JIT to evaluate this.
Suggest doing something like:
int sum = 0;
while(reader.Read())
sum += reader.GetInt32(0);
GC.KeepAlive(sum);Also, I'd suggest forcing this inline and making it static so you can minimise the overhead of the function call itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alsi-lawr, thank you for the review
Let's prevent JIT optimization here. I wrote code with a sum variable, added MethodImplOptions.AggressiveInlining and made it static
|
|
||
| namespace Turso.Native; | ||
|
|
||
| public class TursoNativeDatabase : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these mid-level "native" abstractions are redundant. These do not add actual type safety, that should be done in the core bindings that are being exposed. Any handles you've done in here should just be moved to the actual database abstraction that should be what users consume. No one should be using this, and including it in both the publicly shipped API and the library more generally is confusing (how does a user decide whether to use a TursoValue or a TursoNativeValue?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, these abstractions shouldn't be accessible to the public
So I added a Turso.Raw project, moved all P/Invoke code to that project and made some abstractions like TursoNativeValue internal
Now the Turso project remains only ADO.NET classes
| fixed(TursoErrorHandle* errorHandlePtr = &this) | ||
| return (IntPtr)errorHandlePtr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only fixes the memory address of the error handle in the scope of the fixed block. Once you've left this block, there is no guarantee that the stack address for this value remains fixed. Passing this then to unmanaged code is completely undefined behaviour, and can cause rewrites of arbitrary data on the stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisiting this: stack memory is never relocated, that's my bad. There is no guarantee that this will be stackallocated, however, and is a useless operation. It's still very bad practice to pass the pointer from managed to unmanaged like this when it's unnecessary. Why instantiate in managed c# when you can just as easily just let the argument be an out? Why have 2 steps of instantiating the pointer and passing it to rust just to immediately overwrite the data at that address, when you can just let the unmanaged rust code do it all in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that redunant
Error handling with out Intptr error parameter just simplier, so I rewrote that to recieving errors via out parameter
| private const string DllName = "turso_dotnet.dll"; | ||
|
|
||
| [DllImport(DllName, EntryPoint = "db_open", CallingConvention = CallingConvention.Cdecl)] | ||
| public static extern IntPtr OpenDatabase(string path, IntPtr errorHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very dangerous way of setting data from unmanaged code. .NET already deals with this for you. You can change this signature to
[DllImport(DllName, EntryPoint = "db_open", CallingConvention = CallingConvention.Cdecl)]
public static extern IntPtr db_open(string path, out IntPtr errorHandlePtr); // move this to a static class "TursoInterop" or something along those linesA good structured approach would be to have the raw P/Invoke calls left bare and internal, and absolutely leave the pointer assignments to the unmanaged code. Then have a managed wrapper function for each call that converts it into the C# data types, like:
public static class TursoBindings
{
public static IntPtr OpenDatabase(string path); // this will throw on failed connection
}This can just be used within the actual ADO.NET DbConnection wrapper without a TursoNativeDatabase abstraction, since you're never actually representing the Database struct in the managed C# at all. You just need to pass the handle around (as a SafeHandle -- look into why this is used pls).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, using SafeHandle is much better
Here I wrote a TursoDatabaseHandle and TursoStatementHandle which hold databasePtr and statementPtr
Now TursoBindings wraps raw IntPtr and returns an actual SafeHandle
public static class TursoBindings
{
public static TursoDatabaseHandle OpenDatabase(string path);
}
| [StructLayout(LayoutKind.Explicit)] | ||
| public struct TursoNativeArray | ||
| { | ||
| [FieldOffset(0)] | ||
| public IntPtr Data; | ||
|
|
||
| [FieldOffset(8)] | ||
| public UInt64 Length; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not safe. IntPtr is not guaranteed to be 8 bytes, neither on the rust side *const u8 or on the C# side. It's platform dependent, and will have a different offset on 32 bit and 64 bit platforms. You've hard-coded the offsets for 64 bit only. Just make it sequential, C# will very happily map the byte layout of the rust struct to this struct in a platform-dependent way for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's made it StructLayout(LayoutKind.Sequential)
cd8f6d4 to
8241a10
Compare
Added C#/dotnet bindings for turso